Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Various deriving bitrepr fixes #2209

Merged
merged 5 commits into from
May 18, 2022
Merged

Various deriving bitrepr fixes #2209

merged 5 commits into from
May 18, 2022

Conversation

rowanG077
Copy link
Member

@rowanG077 rowanG077 commented May 12, 2022

This PR fixes the following bit representation derivation issues:

1. Quadratic compilation time when deriving

The process of deriving DataReprAnn has quadratic complexity in the size of constructors and fields.

For example this data type:

data ManyCons
   = ManyCons1  (Index 10) (Index 11) (Index 12)
   | ManyCons2  (Index 11) (Index 12) (Index 13)
   | ManyCons3  (Index 12) (Index 13) (Index 14)
   | ManyCons4  (Index 13) (Index 14) (Index 15)
   | ManyCons5  (Index 14) (Index 15) (Index 16)
   | ManyCons6  (Index 15) (Index 16) (Index 17)
   | ManyCons7  (Index 16) (Index 17) (Index 18)
    
$(deriveAnnotation (simpleDerivator OneHot OverlapR) [t| ManyCons |])
$(deriveBitPack [t| ManyCons |])

Going from 1 constructor to all 7 yields these compilation times on my machine:

  • 1 constructors: 4.84 seconds
  • 2 constructors: 3.31 seconds
  • 3 constructors: 4.43 seconds
  • 4 constructors: 7.31 seconds
  • 5 constructors: 12.06 seconds
  • 6 constructors: 23.03 seconds
  • 7 constructors: 47.78 seconds

The reason is that the TemplateHaskell in deriveAnnotation creates insanely large annotations. See this file where I dumbed the splice(-ddump-splices) for the case with 4 constructors.

This PR fixes this issue by introducing let-bindings of commonly used sub-expressions in the generated annotation. See here for the generated annotation after this PR.

A quick test shows these compilation times after this PR:

  • 1 constructors: 2.55 seconds
  • 2 constructors: 2.61 seconds
  • 3 constructors: 2.67 seconds
  • 4 constructors: 2.87 seconds
  • 5 constructors: 3.11 seconds
  • 6 constructors: 2.85 seconds
  • 7 constructors: 2.92 seconds
  • 32 constructors: 5.03 seconds

2. Promoted data cons and symbols

Promoted data cons and symbols weren't supported at all. Which lead to issues were you used promoted data constructed as a parameter to a type you wanted to derive a bitrepresentation for. Or if you wanted to for example use the domain somewhere in your data type.

3. Type synonyms weren't resolved

During the process of finding types annotation exist for there could be a mismatch because the types coming via GHC core had their type synonyms resolved. While the types coming in via the annotation hadn't. The result was that if you use a type synonym somewhere in the annotation it wouldn't be able to match it with any type in the Custom representation map. Now it always fully resolves that type during the Template Haskell phase at the annotation.

@rowanG077 rowanG077 force-pushed the cse-deriving-bitrepr branch from 03420b0 to 28f62b8 Compare May 12, 2022 17:14
@rowanG077
Copy link
Member Author

rowanG077 commented May 12, 2022

Oh and btw unrelated to this issue. My data type was so large that it takes ages to compile. So I didn't notice this before. But do I need to do something in addition to using deriveAnnotation and deriveBitPack?. When I pack my data type in Haskell it now has a one-hot representation. But the generated Verilog code still uses a non-one-hot encoding.

@christiaanb
Copy link
Member

No, the deriveAnnotation should be sufficient to generate HDL with one-hot encoding. So it sounds like a bug.

@rowanG077 rowanG077 force-pushed the cse-deriving-bitrepr branch 4 times, most recently from d3c8607 to a9870fa Compare May 16, 2022 07:30
@rowanG077 rowanG077 changed the title Cse deriving bitrepr Various deriving bitrepr fixes May 16, 2022
@rowanG077 rowanG077 force-pushed the cse-deriving-bitrepr branch 2 times, most recently from a0f1a7a to 6964085 Compare May 16, 2022 09:22
@rowanG077
Copy link
Member Author

rowanG077 commented May 16, 2022

This is ready for review and possible merge now.

@leonschoorl
Copy link
Member

[...] But do I need to do something in addition to using deriveAnnotation and deriveBitPack?. When I pack my data type in Haskell it now has a one-hot represantation. But the generated Verilog code still uses a non-one-hot encoding.

No, you shouldn't have to do anything.
deriveBitPack is supposed to match any DataReprAnn annotation, no matter if it's written by hand or created via deriveAnnotation.
Please open an issue for that.

@rowanG077
Copy link
Member Author

@leonschoorl I fixed that in this PR this morning, in addition with a few more things.

@leonschoorl
Copy link
Member

Oh nice
Was it caused by the the Type synonyms not being resolved?

@rowanG077
Copy link
Member Author

@leonschoorl That was one of the issues. So the updated top comment of this PR

Copy link
Member

@leonschoorl leonschoorl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some of you commit (titles) are a bit long/wide, causing truncation in some tools, like github.
Specifically:
Clash.Annotations.BitRepresentation.Deriving.deriveAnnotation no lo…
Just dropping the module name would help a lot.

@rowanG077 rowanG077 force-pushed the cse-deriving-bitrepr branch from 6964085 to 2a2d1cf Compare May 17, 2022 14:17
@martijnbastiaan martijnbastiaan merged commit e324601 into master May 18, 2022
@martijnbastiaan martijnbastiaan deleted the cse-deriving-bitrepr branch May 18, 2022 18:53
martijnbastiaan added a commit that referenced this pull request May 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants